-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: rcmgr: Change LimitConfig to use LimitVal type #2000
Conversation
LGTM. I'll get this PR and do some integration on Kubo to see how it looks. |
after a sync conversation with @marten-seemann. 0 in the JSON should mean "block all". This is because a user who manually edits a limit from 1 to 0 would be surprised to see 0 actually mean default instead of block all. |
@MarcoPolo I have a question about how to integrate this when using I had to copy var infiniteBaseLimit = rcmgr.BaseLimit{
Streams: math.MaxInt,
StreamsInbound: math.MaxInt,
StreamsOutbound: math.MaxInt,
Conns: math.MaxInt,
ConnsInbound: math.MaxInt,
ConnsOutbound: math.MaxInt,
FD: math.MaxInt,
Memory: math.MaxInt64,
} limit is a limiter, ok := s.(rcmgr.ResourceScopeLimiter)
if !ok {
return ...
}
baseLimit := limit.Build(infiniteBaseLimit)
limiter.SetLimit(&baseLimit) Shall we set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added small things I saw wen integrating with Kubo here: ipfs/kubo#9612
|
3b3613c
to
56056a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not fully convinced that ConcreteLimitConfig.ToPartialLimitConfig()
is a good idea, but I don't want to block this PR on that.
@MarcoPolo Could you add a paragraph about the rcmgr changes to the v0.25 release issue please?
// DefaultLimit is the default value for resources. The exact value depends on the context, but will get values from `DefaultLimits`. | ||
DefaultLimit LimitVal = 0 | ||
// Unlimited is the value for unlimited resources. An arbitrarily high number will also work. | ||
Unlimited LimitVal = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to make this math.MaxInt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purely preference. A -1 looks different than some large number 9223371036854775807
in case this is printed out. Is that large number a Max Int? (it isn't)
) | ||
|
||
func (l LimitVal) MarshalJSON() ([]byte, error) { | ||
if l == Unlimited { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: a switch
would be nicer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any API breaking changes with this work?
Regardless I didn't see any mention of the changes in https://github.com/libp2p/go-libp2p/releases/tag/v0.25.0
(it makes me think that having changelog files would be useful so that this text gets captured as part of the PR itself. Kubo follows this approach).
// But let me open connections to them | ||
Conns: rcmgr.DefaultLimit, | ||
ConnsOutbound: rcmgr.DefaultLimit, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you even have to specify rcmgr.DefaultLimit or are those just added for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added for clarity, and to show that default doesn't get serialized into json. You'll also notice Memory is implicitly default.
// "StreamsInbound": "blockAll", | ||
// "StreamsOutbound": "unlimited", | ||
// "ConnsInbound": "blockAll" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the order that they serialize as? If we are in control, I think it would be good to go from outwards to in
Conns
ConnsInbound
CoonnsOutbound
Streams
StreamsInbound
StreamsOutbound
This will omit defaults from the JSON output. It will also serialize the | ||
blockAll, and unlimited values explicitly. | ||
|
||
The `Memory` field is serialized as a string to workaround the JSON limitation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To drive this home, maybe set a Memory limit in your example above so it shows up in the serialization?
A `ScalingLimitConfig` can be converted into a `ConcreteLimitConfig` (which can then be | ||
used to initialize a fixed limiter with `NewFixedLimiter`) by calling the `Scale` method. | ||
The `Scale` method takes two parameters: the amount of memory and the number of file | ||
descriptors that an application is willing to dedicate to libp2p. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(minor) Maybe also mention the AutoScale method for added convenience?
This solves the problem around the encoding having different semantics than the config that is passed into the resource manager context. This makes special values explicit (Unlimited, default, block all). And I think it's a more ergonomic API (create a struct with sane defaults, rather than having to copy some pre-initialized struct, then modify relevant values).
The high level idea is you have some custom configuration called
PartialLimitConfig
that gets constructed with aConcreteLimitConfig
(a configuration that has no special or missing values, zero means zero, max means max). The.Build
method fills in any missing information from thePartialLimitConfig
and returns a completeConcreteLimitConfig
. Another way to think about it is thatPartialLimitConfig
is a partial overrides config, and you add it to a completeConcreteLimitConfig
to get anotherConcreteLimitConfig
.Also take a look at the README for some updates on the description there.
Changes:
LimitVal
which can explicitly specify "use default", "unlimited", "block all", as well as any positive number. The zero value ofLimitVal
(the value when you create the object in Go) is "Use default".ResourceLimits
type which usesLimitVal
instead of ints so it can encode the above for the resources.LimitConfig
toPartialLimitConfig
and usesResourceLimits
. This along with the marshalling changes means you can now marshal the fact that some resource limit is set to block all.Before:
After:
And the JSON encoding:
This doesn't affect the ScalingLimitConfig. That has a lot of knobs and represents the developer's knowledge of how the resource usage should scale. LimitConfig on the other hand is more like the manual tweaks an end-user would make. It's used in conjunction with the defaults created from a ScalingLimitConfig.